Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some bugfixes and minor enhancements #36

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

neocturne
Copy link

Hi, I found this nice library just yesterday and used it to write a minimal DNS server. When writing it I found some annoying bugs, so I fixed them :) I hope the commit messages are self-explaining...

It doesn't make much sense to test for a fixed list of TLDs as the list will
change every now and then. Instead add some more checks for syntax correctness,
and allow a trailing dot; also make the method accept the root domain ".".
If the mailbox part of a RNAME contains dots, they have to be masked with
backslashes, making the RNAME an invalid domain name. For such a RNAME to
be recognized as valid add a RNAME validation method that only checks the
domain part for validity.
@weppos
Copy link
Collaborator

weppos commented May 8, 2013

Hello @NeoRaider,

Thanks for your patch. In order to merge the patch, each change must be validated with an unit test. Would you mind to update the patch?

-- Simone

@neocturne
Copy link
Author

Hi @weppos,
at the moment I'm a bit busy, so I don't know when I'll get around to add some tests...

@ghost
Copy link

ghost commented May 12, 2014

I have forked this repository (after attempting and failing to contact author) here: https://github.com/mordocai/net-dns. If you wish to resubmit your pull request there I will review/merge it in. It will shortly be the gem net-dns2 on rubygems.org.

@ghost
Copy link

ghost commented May 13, 2014

Your changes have been merged in my fork here: https://github.com/mordocai/net-dns/commit/1c779bd981e03f4eb10f37069337760aeab8f586. They should be in the release later this week v0.8.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants